Skip to content

support error in context of response - #781

Open
lingdor wants to merge 1 commit into
gocolly:masterfrom
lingdor:response_error
Open

support error in context of response#781
lingdor wants to merge 1 commit into
gocolly:masterfrom
lingdor:response_error

Conversation

@lingdor

@lingdor lingdor commented Sep 17, 2023

Copy link
Copy Markdown

more scene, we don't want stop runing in a page request faild, so we can use onerror log or add retry queue and continue the spider.

@WGH-

WGH- commented Oct 16, 2023

Copy link
Copy Markdown
Collaborator

What's the expected use case? Abort further processing and retry the request if you notice something's wrongin one of the OnResponse handlers (e.g. error page) ?

@StantonMatt StantonMatt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a local pass because this changes the public callback APIs and WGH- asked about the expected use case.

On the stale head f1303d0, these pass:

  • go test . -run TestResponseContextError -count=1 -v
  • go test ./... -count=1
  • git diff --check origin/master...HEAD

The blocker I see is API compatibility. Changing ResponseCallback, HTMLCallback, XMLCallback, ErrorCallback, and ResponseHeadersCallback to return error means existing callback registrations no longer compile. For example, current source passes go test ./_examples/proxy_switcher, but this branch fails with _examples/proxy_switcher/proxy_switcher.go:23:15 because func(r *colly.Response) is no longer assignable to ResponseCallback. The same signature break shows up broadly across checked-in examples such as basic, error_handling, request_context, scraper_server, and shopify_sitemap.

The branch also has content conflicts with current master in colly.go, colly_test.go, and http_backend.go.

Given that every existing callback user would need to add a return value, I do not think this shape is mergeable as a v2 change unless maintainers intentionally want a breaking API. A smaller path may be a separate opt-in hook for handlers that want to return an error, while keeping the existing callback types source-compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants